fix(cmake): prioritize discovered zlib headers for brpc sources - #3475
Conversation
zchuango
changed the title
There was a problem hiding this comment.
Pull request overview
This PR addresses a CMake integration issue when bRPC is consumed via add_subdirectory(): inherited include directories from a parent project can cause Protobuf’s gzip_stream.h to pick up an unrelated zlib.h (e.g., Crypto++), breaking compilation. The changes make zlib discovery explicit and attempt to prioritize the intended zlib headers during compilation of bRPC sources.
Changes:
- Add
find_package(ZLIB REQUIRED)and link againstZLIB::ZLIBinstead of the barezlibrary name. - Prepend the discovered zlib include directory(ies) for the
SOURCES_LIBobject library to avoid header collisions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| CMakeLists.txt | Explicitly discovers zlib and links with ZLIB::ZLIB to use CMake’s standard zlib integration. |
| src/CMakeLists.txt | Adjusts include ordering for SOURCES_LIB so Protobuf’s gzip compilation resolves the intended zlib.h. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| # protobuf/io/gzip_stream.h includes <zlib.h>. Prioritize the discovered | ||
| # zlib headers over include directories inherited from parent projects. | ||
| target_include_directories(SOURCES_LIB BEFORE PRIVATE ${ZLIB_INCLUDE_DIRS}) |
There was a problem hiding this comment.
This should be addressed at the parent project level by scoping the include paths to the intended modules, rather than modifying brpc's internal header order.
|
@wasphin I’m not sure I follow. Are you suggesting that this should be fixed entirely in the parent project, with no changes needed in bRPC? |
|
@zchuango Adding find_package(ZLIB REQUIRED) and linking against ZLIB::ZLIB is reasonable because zlib is a direct dependency of bRPC. My concern is specifically about changing SOURCES_LIB include ordering to work around include directories leaked from the parent project. bRPC cannot guarantee that its dependency headers will never conflict with unrelated headers exposed by a parent project, and prioritizing the zlib include directory only addresses this particular collision rather than the underlying dependency-scope issue. The parent project should scope each module dependency with target-level CMake commands, such as target_include_directories(), instead of exposing unrelated include directories through directory-level settings. This prevents unnecessary dependencies from leaking into bRPC compilation scope. Therefore, I do not think bRPC should work around the parent project dependency leakage by changing its internal include order. |
I agree that bRPC should not adjust its internal include ordering to compensate for include directories leaked by a parent project. I have removed the After removing the workaround, I reran the original parent-project collision reproduction. As expected, the build again failed with I have updated the PR description to make this limitation explicit. All GitHub Actions checks are now passing. |
What problem does this PR solve?
Issue Number: #2593
Problem Summary:
zlib is a direct dependency of bRPC, but the CMake build currently links it using the bare library name
zwithout explicitly discovering the dependency.This makes the CMake dependency declaration less portable and prevents CMake from carrying the configuration provided by
FindZLIB, such as a toolchain-specific library selection.The problem was observed while investigating #2593, where bRPC was integrated into a parent project with
add_subdirectory. However, include directories leaked by a parent project should be scoped at the parent-project level and are not worked around by this change.What is changed and the side effects?
Changed:
find_package(ZLIB REQUIRED)to discover zlib explicitly.zlink dependency with the standard CMake imported targetZLIB::ZLIB.This makes bRPC's direct zlib dependency explicit and allows CMake to use the zlib library selected by the active toolchain or package configuration.
This PR does not modify global or target include ordering. Parent projects remain responsible for scoping unrelated include directories with target-level CMake commands such as
target_include_directories.Side effects:
Verification
Standard builds:
Build and Test on Linux: passed.Build on Macos: passed.License Check: passed.Scope validation:
After removing the
SOURCES_LIBinclude-order workaround, the original parent-project collision was tested again with:/workspace/cryptoppadded as a directory-level include path by the parent project;add_subdirectory;REPRO_CRYPTOPP_ZLIB_COLLISION=ON;CMake configuration succeeded, but compilation failed at step 213/378. The compile command placed the parent include directory before the bRPC source directory:
Protobuf's gzip_stream.h then selected Crypto++'s unrelated zlib.h and failed with:
error: 'z_stream' does not name a type
This confirms that linking through ZLIB::ZLIB improves dependency modeling but does not correct include directories leaked by a parent project. Following the review feedback, that collision is considered a parent-project dependency-scoping issue and is intentionally not worked around by this PR.
Check List: